-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add video support in Grid Bid Adapter #3545
Conversation
} else { | ||
bidResponse.ad = serverBid.adm; | ||
bidResponse.mediaType = BANNER; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi this all looks great,
I have a question as to why when it is video there is no bidResponse.width
or bidResponse.height
added?
I see it is not sent or returned by your adserver.
You can see in the below screenshot that the KVP that gets set for the size turns out to be undefinedxundefined
I have a feeling this may cause problems for certain publishers ad servers maybe?
Do you think it is a good idea to set the width and height based on the bidRequest object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Prebid Docs:
http://prebid.org/dev-docs/bidder-adaptor.html#interpreting-the-response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width and height are required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for the catch, we have updated code, please review again.
} | ||
} | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for good test parameters which return valid bid responses!
This is great.
'creativeId': 1, | ||
'dealId': undefined, | ||
'width': undefined, | ||
'height': undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above question.
Is there a reason why the adapter does not just set these values?
@@ -163,6 +163,7 @@ describe('TheMediaGrid Adapter', function () { | |||
'ad': '<div>test content 1</div>', | |||
'bidderCode': 'grid', | |||
'currency': 'USD', | |||
'mediaType': 'banner', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for > 80% Test Coverage! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Great Stuff @TheMediaGrid !
Type of change
Description of change
Added video support in Grid Bid Adapter
grid-tech@themediagrid.com
official adapter submission
A link to a PR on the docs repo Video support in Grid doc file prebid.github.io#1149